Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mlo 133 create admission controller #906

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

parikls
Copy link

@parikls parikls commented Feb 12, 2025

No description provided.

Copy link

linear bot commented Feb 12, 2025

Copy link

gitguardian bot commented Feb 12, 2025

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@github-actions github-actions bot added the review-ready PR is ready for the review label Feb 12, 2025
@parikls parikls marked this pull request as draft February 12, 2025 10:41
@github-actions github-actions bot removed the review-ready PR is ready for the review label Feb 12, 2025
@parikls parikls force-pushed the MLO-133-create-admission-controller branch from e7451fe to 2767261 Compare February 12, 2025 11:21
@parikls parikls requested a review from zubenkoivan February 12, 2025 14:16
@parikls parikls marked this pull request as ready for review February 12, 2025 14:17
@github-actions github-actions bot added the review-ready PR is ready for the review label Feb 12, 2025
MLO-133: charts
LABEL_APOLO_ORG_NAME = "platform.apolo.us/org"
LABEL_APOLO_PROJECT_NAME = "platform.apolo.us/project"
LABEL_APOLO_STORAGE_MOUNT_PATH = "platform.apolo.us/storage.mountPath"
LABEL_APOLO_STORAGE_HOST_PATH = "platform.apolo.us/storage.hostPath"
Copy link
Author

@parikls parikls Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zubenkoivan two questions here:

  • since this will be a single label of a JSON type, I think we can support multiple mounts?
  • WDYT about a name platform.apolo.us/injectStorage ? or platform.apolo.us/storage.Inject ?

Copy link
Contributor

@zubenkoivan zubenkoivan Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use platform.apolo.us/inject-storage

- add label selector for admission controller
- use new secrets
- refactoring of a webhook and an admission controller
Copy link
Contributor

@zubenkoivan zubenkoivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add tests for storage injection, may be in a follow up pr, up to you

subjects:
- kind: ServiceAccount
name: {{ .Values.admissionController.app_name}}
namespace: platform
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
namespace: platform
namespace: {{ .Release.Namespace }}

subjects:
- kind: ServiceAccount
name: {{ .Values.admissionController.app_name }}
namespace: platform
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
namespace: platform
namespace: {{ .Release.Namespace }}


context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)

crt_file = tempfile.NamedTemporaryFile(mode="w", delete=False, suffix='.crt')
Copy link
Contributor

@zubenkoivan zubenkoivan Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use it also as a context manager instead and pass delete=True, delete_on_close=False

value=[]
)

for mount_path, storage_path in injection_spec.items():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also support volume mount modes (readonly, readwrite). We support them in cli

future_volume_name = create_injection_volume_name()

# add a volume host path
admission_review.add_patch(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be multiple volumes. For example user wants to mount storages of different orgs and copy data between them.

kind: Deployment
metadata:
name: {{ .Values.admissionController.app_name }}
namespace: platform
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not define namespace, helm by default installs resources in the namespace where helm release is created

kind: Service
metadata:
name: {{ .Values.admissionController.app_name }}-svc
namespace: platform
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove namespace



admissionController:
app_name: "storage-admission-controller"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove it and use generated name

labels.get("app") == LABEL_PLATFORM_STORAGE_POD and
labels.get("service") == LABEL_PLATFORM_STORAGE_POD
):
return await self._handle_new_platform_storage_pod(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that we could mount all volumes to admission controller. In this case we won't need to listen to changes in platform storage pods, it will have the latest available volumes. We will only need to load volumes of the pod hosting this service during startup. wdyt?

failurePolicy: Fail
objectSelector:
matchLabels:
platform.apolo.us/storage-injection-webhook: "enabled"
Copy link
Contributor

@zubenkoivan zubenkoivan Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
platform.apolo.us/storage-injection-webhook: "enabled"
platform.apolo.us/inject-storage: "true"

wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-ready PR is ready for the review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants